Editorial: have CreateIteratorFromClosure provide a new execution context to GeneratorStart #2398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmdyck
previously requested changes
Apr 29, 2021
|
I suppose an alternative is to have GeneratorStart create an execution context itself, and never push it to the execution context stack. That has the advantage that it would not be manipulating the code evaluation state of the running execution context, but the disadvantage that there would then be two execution contexts created for a generator: one for evaluating the parameters, created by [[Call]], and one for evaluating the body, created by GeneratorStart. |
syg
approved these changes
Jun 9, 2021
bakkot
added
spec bug
and removed
editor call
michaelficarra
approved these changes
Jun 11, 2021
ljharb
force-pushed
the
master
branch
3 times, most recently
from
3d0c24c
to
7a79833
Compare
bakkot
added
the
ready to merge
…tion context to GeneratorStart (#2398)
mathiasbynens
pushed a commit
to mathiasbynens/ecma262
that referenced
this pull request
Oct 18, 2021
…tion context to GeneratorStart (tc39#2398)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See more complete discussion in https://es.discourse.group/t/execution-context-suspension-and-resumption/756. I'll summarize:
GeneratorStart manipulates the code evaluation state of the running execution context, and also stores that context on the generator object. Prior to #2045, the sole caller of GeneratorStart was in EvaluateGeneratorBody (although it wasn't called that at the time). So, in particular, GeneratorStart was only invoked in step 7 of [[Call]] for ordinary functions, at which time the running execution context would have been one created specifically for the evaluation of the generator in step 3 of [[Call]] - that is, the execution state being manipulated and stored would have been created specifically for the generator. And it would be removed from the execution context stack immediately after the call to GeneratorStart (in step 8 of [[Call]]) (and so implicitly suspended), so it makes sense to be describing what should happen after resumption at this point.
But #2045 introduced a new callsite for GeneratorStart, CreateIteratorFromClosure. Here the execution context is not created for the "generator"; it's just whatever happens to be running at the time. This doesn't really make sense - you don't want to be manipulating the execution state of the caller of CreateIteratorFromClosure!
The fix, I believe, is to have CreateIteratorFromClosure create a push a new context before calling GeneratorStart, and pop it after. That way it can "belong" to the synthetic generator it's creating. That is what this PR does.
Note to self: this needs to address CreateAsyncIteratorFromClosure in the same way.